-
Notifications
You must be signed in to change notification settings - Fork 453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: ✨ Official docker images for docTR #1322
feat: ✨ Official docker images for docTR #1322
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1322 +/- ##
==========================================
+ Coverage 95.78% 95.79% +0.01%
==========================================
Files 154 154
Lines 6902 6902
==========================================
+ Hits 6611 6612 +1
+ Misses 291 290 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
748a82b
to
dc86d0d
Compare
Further todo's:
@odulcy-mindee Anything missing ? ^^ |
That's good ! 👍 |
7dcd8e9
to
51dd73d
Compare
This reverts commit 51dd73d.
@felixdittrich92 FYI:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@odulcy-mindee Really good job that's a great and useful feature 👍🏼
I added only a few comments and questions
|
||
- name: Push Docker image | ||
# Push only if the CI is not triggered by "PR on main" | ||
if: github.ref == 'refs/heads/main' && github.event_name != 'pull_request' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not 100% sure that this works on publish (release)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixT2K Mmmh, actually it works for tags
but it's not done on publish: [released]
as I'm afraid the CI job metadata
from docker
does not handle this event. I'll double check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixT2K I did a test on my fork. When a draft release is created, a new tag is not created. When the release is published, the tag is created, so it triggers the on: push: tags
event 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice :)
pull_request: | ||
branches: main | ||
schedule: | ||
- cron: '0 2 29 * *' # At 02:00 on day-of-month 29 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixT2K I changed the cron so it should be triggered tomorrow morning. We'll see if it works or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alright 👍🏼
great job 🤗 thanks |
🚀 |
where are the prebuilt docker images hosted? |
Having reviewed your Dockerfile, it seems that it's configured to operate as root by default. This is a significant security risk and is likely unacceptable for most production environments. |
Hello @ffalkenberg,
You can find the docker images here. You can also have a look at this section in the README.md
Yes, the container runs as root, as it did before. I understand your concern. For that, I suggest you take a look at what Jupyter has done, they have such an interesting approach to handling that, but it's really not trivial. This deserves a dedicated Pull Request to handle these user/permission issues. |
It's
It's a folder. Moreover, this works:
So it's not a permission issue. For the demo app, you can still craft your own Docker image, based on the previous modification: FROM ghcr.io/mindee/doctr:torch-py3.9.18-gpu-2023-09
ENV DOCTR_CACHE_DIR=/app/.cache
WORKDIR /app
COPY . .
RUN chmod -R a+w /app You can also add a dedicated user in the container but it's out of scope of this PR. |
TODO
Still heavy TBH: